Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new ruff-format-docs hook #112

Closed
wants to merge 35 commits into from
Closed

Conversation

calumy
Copy link
Contributor

@calumy calumy commented Dec 30, 2024

Note: #111 has been split out of this PR to allow for an independent review of the changes. I am unable to point the base branch to the branch of that PR, as this causes the PR to be created in the forked repo. I hope that #111 can be reviewed and merged first, and then this PR is rebased on top of the changes; however, I have opened this PR to show the utility of some of the hooks added in that PR, e.g. mypy or some of the more opinionated ruff rules.

Summary

This PR introduces a new hook to run the ruff formatted over code blocks in documentation files. This was built on top of the Blacken-docs project but extended for the ruff formatter. I think this hook should be included in this repo rather than Blacken-docs, as it allows the hook to be versioned with the correct version of ruff and automatically updated when a new version of ruff is added rather than relying on manual updates of the ruff version (as is currently implemented with black in blacken-docs).

Naming

Initially, I had thought of this hook as ruffen-docs; however, I believe ruff-format-docs aligns better with the other hooks in this repo and also leaves open the door for a ruff-check-docs hook in the future.

Licencing

The Blacken-docs repo is licenced under an MIT Licence, so I believe that using it as a starting point is covered by this. I have also included a reference to the Blacken-docs repo in the readme.

Test Plan

  • Test cases ported from blacken-docs
  • Try with try-repo in another repo that has code blocks in documentation e.g. the main ruff repo. uv run pre-commit try-repo <path-to-pre-commit-ruff> ruff-format-docs --files crates/**/resources/mdtest/*.md
    • Main changes introduced here are the addition of extra white blank lines before function definitions
    • Note some files fail to parse, but these are due to invalid syntax in the files. These files are excluded by the current blacken-docs config in this repo.

@MichaReiser
Copy link
Member

MichaReiser commented Jan 10, 2025

Wow, this is great. Thank you for working on this.

We discussed this internally and concluded that this isn't how we want to enable markdown formatting support in Ruff:

  • There's an open Blacken Docs issue about adding Ruff support (Support Ruff formatter as an alternative for Black adamchainz/blacken-docs#352). There are performance concerns because it requires spawning a new Ruff process for every code snippet. I assume the same would apply to your implementation. This is sort of a show-stopper because performance is a large aspect of our brand.
  • We aim for an end-to-end solution for users: You can use Ruff in your editor, with pre-commit, and on the CLI. A pre-commit-only solution doesn't fit well into this architecture.

This doesn't mean that a pre-commit wouldn't be useful for others. We might actually want to use it to format our test files. However, we don't think it fits nicely under the Astral brand. I encourage you to consider releasing the pre-commit under your own account. We could also try to integrate this into Blacken Docs by working on the Python API and than PR the Ruff support to Blacken docs. Maybe that's something you'd be interested in? As a heads up, we're also considering building this directly into Ruff's CLI.

Thanks again for the great work and I think a pre-commit like this would be a great interim solution for users wanting to use Ruff to format markdown files. Let me know if you decide to publish it under your own name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants